Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Latest Comments Block: Remove list padding and margin #29402

Merged
merged 4 commits into from
Mar 26, 2021

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Feb 27, 2021

Description

Latest Comments Block: Remove list padding and margin. Fixes #26858

How has this been tested?

Added a latest comments block.
Confirmed that the block aligns with the rest of the content in the editor and front (depending on alignment).

Screenshots

The block is aligned with the rest of the content:

image

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Latest Comments Block: Remove list padding and margin.
@carolinan carolinan requested a review from ajitbohra as a code owner February 27, 2021 06:09
Copy link
Member

@ajitbohra ajitbohra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@@ -1,3 +1,8 @@
.wp-block-latest-comments {
padding-left: 0;
margin-left: 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with TT1 blocks and this change is not reflected in the editor. Also in front-end the margin-left is overridden with auto and if it wasn't it would place this block at the left of the page differently from other blocks that would retain margin-left:auto.

So maybe patch the editor and padding-left should be enough 🤔 . I'm not sure how other themes behave to this though..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I should not have tested with Twenty Twenty-One 🤦

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

The padding change is great, but the margin change, I believe, will cause headaches. Many themes, to Nik's point, center blocks using margin-left: auto, and by setting it to zero, you might make the latest comments block bump againt the left edge.

We have the same challenge with the Image block, which is wrapped in a figure element. The figure is naturally born with left and right margins that would be nice to zero out. But we don't, exactly because we don't want to break that centering mechanism.

For that reason, I'd leave the margin change out from this PR, and let the theme handle that. Then, longer term, something like #29335 could potentially help address the fundamental issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The margin rule has been removed from the front but not the editor.
In the editor, the margin only affects the list, not the actual block position, since the list is wrapped. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the front and backend ideally be the same though? Or does the ol inherit from wp admin which diverges it anyway? 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes 🙊 , the list styles we are overriding are:
Browser defaults
common.css (wp admin)
and the Editor Normalization Styles

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

I think it might still be best to remove that margin change, as it "paints over" an issue that we need to fix at the root, for example through bringing the iframe to the post editor, or #28878.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♀️ It will look different in the editor and front even without the wp admin styles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♀️ It will look different in the editor and front even without the wp admin styles.

This is no different from what's currently in trunk, though, is it? I definitely agree it needs to be addressed, just want to make sure we do it right.

@ntsekouras ntsekouras requested a review from jasmussen March 1, 2021 12:18
Base automatically changed from master to trunk March 1, 2021 15:46
@skorasaurus skorasaurus added the [Block] Latest Comments Affects the Latest Comments Block label Mar 2, 2021
@carolinan carolinan requested a review from youknowriad March 23, 2021 11:29
@@ -1,3 +1,7 @@
.wp-block-latest-comments {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just use ol.wp-block-latest-comments here and avoid any specific editor style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand. Did you mean to add this comment to the editor.scss file?
That might be possible now after #30034.
I will test it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I mean replace the selector above with ol.wp-block-latest-comments and avoid any style in "editor.scss". It worked in my testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ol.wp-block-latest-comments is still overridden by

.editor-styles-wrapper ol, .editor-styles-wrapper ul {
    margin: revert;
    padding: revert;
}

that is why ol.wp-block-latest-comments.wp-block-latest-comments was used in the editor style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only overridden if the block is part of the post when it gets loaded in the editor. this is an issue with the lazy loading behavior of block assets in the editor. It affects more blocks than just the comments block I believe we should fix the root issue (stylesheet loading order) not its consequences. I already talked with @aristath and @gziolo about it.

@youknowriad youknowriad added the [Type] Enhancement A suggestion for improvement. label Mar 26, 2021
@carolinan carolinan merged commit f1d9a0b into WordPress:trunk Mar 26, 2021
@github-actions github-actions bot added this to the Gutenberg 10.4 milestone Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Comments Affects the Latest Comments Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest Comments Block: Block includes unnecessary left margin + padding.
6 participants